REQUEST CHANGES — two findings:

1. Bug: alerts counter never incremented (price-alerts/route.ts:47)
const alerts = 0; — declared as const and never incremented, but returned in the response. Should be let alerts = 0; and incremented after each successful checkPriceChangeAlert() call. Currently the API always returns { checked: N, alerts: 0 } regardless of how many alerts fire.

2. Missing wallet address validation (save-token/route.ts)
walletAddress from the request body is passed through to the DB with no format validation. Since this is a system boundary (user input → DB), it should validate that the value is a valid Ethereum address (e.g. /^0x[0-9a-fA-F]{40}$/) before storage. Invalid values would cause silent failures in the downstream balanceOf multicall.

Otherwise solid: multicall token holder targeting with fallback is clean, migration is well-structured with proper indexes, price snapshot comparison logic is correct.

Verdict: REQUEST CHANGES

Summary

The notification targeting direction is right, but the current implementation still has one behavior that broadcasts beyond holders and one trust boundary issue in the holder-mapping flow.

Findings

• [high] The new-plot path still broadcasts to all enabled users when no holder notification tokens are found, which violates the targeted-only acceptance criteria.
  • File: lib/notifications.server.ts:272
  • Suggestion: do not fall back to getEnabledTokens() for holder-targeted plot notifications; if no subscribed holders are found, skip delivery instead of broadcasting.
• [high] walletAddress is accepted directly from the client and persisted as the holder-mapping key without any verification, so a caller can register someone else's wallet and receive that wallet's holder-only alerts.
  • File: src/app/api/notifications/save-token/route.ts:10
  • Suggestion: derive the wallet from trusted Farcaster / miniapp context or another verified source instead of trusting raw request JSON.

Decision

Requesting changes because issue #521 explicitly requires new-plot and price-change notifications to target token holders only. The current fallback-to-all path breaks that behavior, and the unverified walletAddress input makes the targeting list spoofable.

Re-reviewed — both T2b findings fixed:

1. alerts counter now uses let and increments correctly (checkPriceChangeAlert returns boolean)
2. walletAddress validated with /^0x[0-9a-fA-F]{40}$/ regex, invalid values silently dropped

APPROVE from T2b.

Verdict: REQUEST CHANGES

Summary

Re-checking against the live issue scope, the current implementation still misses two explicit targeting requirements.

Findings

• [high] The new-plot notification path still broadcasts beyond holders.
  • File: lib/notifications.server.ts:272
  • Suggestion: remove the getEnabledTokens() fallback for holder-targeted plot notifications; if there are no subscribed holders, skip delivery instead of broadcasting.
• [high] The holder mapping still trusts client-provided wallet identity instead of a trusted context-derived mapping.
  • File: src/app/api/notifications/save-token/route.ts:10
  • Suggestion: derive the wallet from trusted Farcaster / miniapp context as described in issue #521, rather than persisting whatever wallet the client submits.

Decision

Requesting changes because the live issue still says: targeted notifications should go to users who hold the storyline token instead of broadcasting to all users, and the suggested wallet mapping source is sdk.context.user / a trusted Farcaster mapping. The current fallback-to-all behavior and raw client wallet mapping still do not meet that scope.

Re-reviewed after T2a blocker fixes. Both addressed:

1. No broadcast fallback — notifyNewPlot returns early if no holders, no getEnabledTokens() fallback
2. Trusted wallet — resolved server-side via Neynar verified_addresses, walletAddress removed from save-token route

APPROVE from T2b.

Verdict: APPROVE

Summary

The current head addresses the two in-scope blockers from my prior reviews. New-plot notifications are now holder-only with no broadcast fallback, and wallet mapping is resolved server-side from trusted Farcaster/Neynar data instead of client-supplied input.

Findings

• None.

Decision

Approving because the current implementation now matches issue #521's targeting requirements: holder-only delivery for new-plot and price alerts, plus trusted wallet resolution for FID-to-wallet mapping.